-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Loki: Override distributor's default ring KV store #4440
Loki: Override distributor's default ring KV store #4440
Conversation
e068f84
to
e5045f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DylanGuedes this looks good, will conflict with #4435 but should be easy to resolve. If your ready to publish I think it's good to go.
Awesome! I added an entry to the CHANGELOG and to the upgrading guide so now it is ready for review. :D edit: and yeah, once #4435 gets merged I'll rewrite the tests on this one |
58ff6bc
to
b1a43a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Dylan!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super nice! 🎉 Thanks @DylanGuedes LGTM!
@DylanGuedes I'll merge this once the conflicts have been resolved |
Looks like @slim-bean have some opinion on it. We will hold off the merge! |
91df8e7
to
04e0dbb
Compare
- Currently, the default KVstore is consul. This changes it to inmemory to make it easier for new users to run the project without dependencies. - Refactor default flag value tests. Now, it will check for a populated map instead of checking for flags during iteration.
04e0dbb
to
d0a1a9b
Compare
Note: I rewrote the commit history because it was becoming weird after rebasing with the latest changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What this PR does / why we need it:
Changes the default distributor ring KV store from
consul
toinmemory
. This change makes the Distributor easier to run with default configs by not requiring Consul.Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
Checklist